GridFS download test reenablement#1991
Conversation
- Remove JAVA-5839 skip entry for "timeoutMS applied to entire download, not individual parts" test so it runs in CI JAVA-5839
Rename dir/file/test to directory/fileDescription/testDescription to clarify that 'file' refers to the spec file's description field, not the filename, and 'test' refers to the individual test description. JAVA-5839
There was a problem hiding this comment.
Pull request overview
Re-enables the unified spec test covering CSOT behavior for GridFS downloads (JAVA-5839) by replacing a hard skip with a targeted test-data transformation, and clarifies naming in the unified test modification helpers.
Changes:
- Removed the JAVA-5839 skip for the GridFS download CSOT spec test and replaced it with a transformation that relaxes timeouts to reduce CI flakiness.
- Renamed
dir/file/testidentifiers inUnifiedTestModificationstodirectory/fileDescription/testDescriptionfor clarity and consistency with spec terminology. - Added a transformation mechanism (
TestTransformer) and integrated it into the unified test runner setup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java | Adds test-data transformation support, renames identifiers for clarity, and applies a transformation for the GridFS CSOT test. |
| driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java | Applies registered transformations during test setup before entity initialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ation Add TestTransformer functional interface and transform() method to TestDef, allowing spec test data (entities and definition) to be mutated before execution. Transformations are logged with a mandatory reason string. Apply transform for JAVA-5839: bump timeoutMS from 75 to 250 and blockTimeMS from 50 to 200 for the GridFS download timeout test to avoid CI latency failures. The 150ms margin (6x the original 25ms) should be reliable across CI environments. JAVA-5839
nhachicha
left a comment
nhachicha
left a comment
There was a problem hiding this comment.
Isn't introducing a TestTransformer risk deviating from the spec tests? in this case if the timeout is too short shouldn't we modify the upstream spec test instead?
AI review (Opus 4.8 1M)
🟡 [important] L271-273: definition is mutated in place while entitiesArray is defensively cloned.
The guard clones entitiesArray before transforming it, but passes the raw definition to applyTransformations, which mutates definition.getArray("operations")...blockTimeMS in place. definition is
later re-read at execution time in shouldPassAllOutcomes() — so the in-place mutation is required there — but the same definition object is reused across retry attempts. This is safe today only because
findAndSetInt is idempotent (it sets a fixed value). It becomes a latent corruption bug the moment any future transformer increments or otherwise depends on the prior value. Either clone definition too
and read the clone downstream, or add a comment documenting that transformers must be idempotent because definition is intentionally shared.
driver-sync/.../unified/UnifiedTestModifications.java
-
🟡 [important]
findAndSetInt: Silent no-op when the path matches nothing.
If zero elements match (a typo in the path, or a future spec YAML renaming/retyping the field), the method completes silently with no substitution and no log line — theLOGGER.infoonly fires on a
successful replacement. Since the whole point is to suppress CI flakiness, a silent miss would quietly reintroduce the original flaky failures with no diagnostic signal. Track a substitution count and log a
warning (orfail) when it stays zero. -
🟢 [nit]
isInt32()guard: A value parsed asBsonInt64/BsonDoubleis silently skipped. Current spec values are int32, so this works, but a one-line comment on why only int32 is accepted (or relaxing
toisNumber()+asNumber().intValue()) would prevent a future silent miss. -
🟢 [nit]
findAndSetIntsearches only top-level array elements, not recursively. Correct for the flatoperationslist today, but the generic name/signature implies otherwise. Consider noting in the
Javadoc that it does not recurse into nestedoperations(e.g.loopoperations). -
💡 [suggestion]
onMatch(transformer path): When a transformer matches,onMatchalso runsthis.testDef.matchesThrowable = this.matchesThrowable, which isnullfor the transformer constructor. If
a test ever registers both atransform(...)and aretry(...)/when...with amatchesThrowable, registration order could null it out. Not triggered by current usage, but worth guarding (assign only when
non-null) if transforms and retries are later combined on one test.
</details?
| * @param path dot-separated path to an int field | ||
| * @param newValue the replacement value | ||
| */ | ||
| static void findAndSetInt(final BsonArray array, final String path, final int newValue) { |
There was a problem hiding this comment.
is there a value in asserting the expected original value? this will make sure if the upstream test changes to a more favourable value maybe the transformation is no longer needed ?
There was a problem hiding this comment.
Done — added alongside the reason change. findAndSetInt now asserts expectedValue before replacing; if the spec changes the value, the error message explicitly says to update the transformation.
| * @param path dot-separated path to an int field | ||
| * @param newValue the replacement value | ||
| */ | ||
| static void findAndSetInt(final BsonArray array, final String path, final int newValue) { |
There was a problem hiding this comment.
can we generalise this method for all numbers? and use isNumber below?
There was a problem hiding this comment.
The implementation already uses isNumber() / asNumber().intValue() — no change needed here.
| } | ||
| } | ||
| String leafKey = segments[segments.length - 1]; | ||
| if (found && current.containsKey(leafKey) && current.get(leafKey).isInt32()) { |
There was a problem hiding this comment.
The transformation is skipped if the value is BsonInt64 ? (maybe related to https://github.com/mongodb/mongo-java-driver/pull/1991/changes#r3356666027)
| * @param path dot-separated path to an int field | ||
| * @param newValue the replacement value | ||
| */ | ||
| static void findAndSetInt(final BsonArray array, final String path, final int newValue) { |
There was a problem hiding this comment.
Can we refactor this to include the reason ? this will make the log statement below (LOGGER.info) more highly informative
Ex:
String reason = "JAVA-5839: Bump blocking/timeout to avoid CI latency failures";
def.transform(reason,
(entitiesArray, definition) -> {
findAndSetInt(reason, entitiesArray, "client.uriOptions.timeoutMS", 250);
findAndSetInt(reason, definition.getArray("operations"), "arguments.failPoint.data.blockTimeMS", 200);
})Or infer it from TestApplicator
// TestDef ...
public void applyTransformations(final BsonArray entitiesArray, final BsonDocument definition) {
for (Transformation t : transformations) {
t.transformer.transform(t.reason, entitiesArray, definition);
}
}
// lambda
def.transform("JAVA-5839: Bump blocking/timeout to avoid CI latency failures",
(reason, entitiesArray, definition) -> {
findAndSetInt(reason, entitiesArray, "client.uriOptions.timeoutMS", 250);
findAndSetInt(reason, definition.getArray("operations"), "arguments.failPoint.data.blockTimeMS", 200);
})There was a problem hiding this comment.
Done — findAndSetInt now takes reason as its first parameter (included in log output and error messages) plus an expectedValue parameter that asserts the original spec value before replacing it. The call sites are updated accordingly.
It does, however the spec's intent is preserved — blockTimeMS × chunks still exceeds timeoutMS. The 75/50ms values are simply too tight for CI latency with Java build nodes; no other driver needs these changed. This PR follows the same philosophy as the existing retry() entries for CSOT RTT variance. |
- findAndSetInt: use isNumber() instead of isInt32() to accept any numeric BSON type, and fail if no replacements are made (surfaces path typos and upstream spec changes) - Guard matchesThrowable assignment in onMatch() to avoid nulling it out when a transformer applicator fires after a retry registration - Clone the definition in setUp and read it from the field in shouldPassAllOutcomes: the injected parameter (oriDefinition) is the original, untransformed instance, so definition-level transforms must be read back from this.definition for them to reach execution
nhachicha
left a comment
nhachicha
left a comment
There was a problem hiding this comment.
LGTM 👍 minor Fable nits
1. Fragile field-shadowing in setUp (UnifiedTest.java:270-273). After renaming the params to oriEntitiesArray/oriDefinition, the unqualified entitiesArray.clone() / applyTransformations(entitiesArray, definition) now silently read the fields. It works (the fields were just assigned, then reassigned to clones), but it's easy to break in a refactor and hard to read. Prefer explicit this. everywhere, or locals:
this.entitiesArray = oriEntitiesArray.clone();
this.definition = oriDefinition.clone();
testDef.applyTransformations(this.entitiesArray, this.definition);2. Same pattern in shouldPassAllOutcomes (UnifiedTest.java:366-368). The local BsonDocument definition = this.definition; plus the explanatory comment is a workaround for the shadowing hazard. Simpler and safer: drop the definition parameter usage entirely (it's already oriDefinition + @SuppressWarnings("unused")) and just reference this.definition — or keep the local but consider whether the comment/rename churn is needed at all given nothing uses oriDefinition.
3. ori prefix is cryptic. The PR aims to clarify naming, but oriEntitiesArray/oriDefinition/oriModifiersToApply reads as a typo. originalDefinition etc. would be self-explanatory.
4. Wrong logger namespace (UnifiedTestModifications.java). org.bson.diagnostics.Loggers.getLogger("UnifiedTestModifications") produces logger category org.bson.UnifiedTestModifications — misleading for driver-sync test code. Project convention is SLF4J (per AGENTS.md); or at least com.mongodb.internal.diagnostics.Loggers.
5. Silent behavior change in onMatch. testDef.matchesThrowable = matchesThrowable is now guarded by a null check. Previously a later matching applicator without whenFailureContains would clear an earlier one; now the earlier one survives. This looks like an intentional fix, but it changes retry-matching semantics for any test with multiple matching applicators — worth a sentence in the PR description or a comment.
6. Transformer wrapper can convert aborts into failures. The lambda in onMatch catches RuntimeException and rethrows as AssertionFailedError. TestAbortedException is a RuntimeException, so a transformer that (indirectly) aborts would be reported as a failure. Unlikely in practice; catching would be safer as-is only if documented.
7. findAndSetInt limitations are documented but note: if the path exists but the leaf isn't numeric, it's skipped silently and only caught by the replacements == 0 check when no other element matches — acceptable for test infrastructure.
This PR comes in 3 parts:
JAVA-5839